fix: Asynchronous burn reentrancy bug #17
Conversation
|
Opening for discussion, currently untested. |
| @@ -109,8 +114,7 @@ abstract contract ConfidentialERC20 is Ownable, IConfidentialERC20, IERC20Metada | |||
| function transfer(address to, euint64 value) public virtual returns (bool) { | |||
| require(TFHE.isSenderAllowed(value)); | |||
| address owner = _msgSender(); | |||
| ebool isTransferable = TFHE.le(value, _balances[msg.sender]); | |||
There was a problem hiding this comment.
I did not like this essential, universal check being the responsibility of the caller, so it is moved to _transfer and the logic is centralised in _hasSufficientBalance which also considers the _lockedBalances
| BurnRq memory burnRequest = burnRqs[requestID]; | ||
| address account = burnRequest.account; | ||
| uint64 amount = burnRequest.amount; | ||
| if (!decryptedInput) { | ||
| revert("Decryption failed"); |
There was a problem hiding this comment.
This was a bit of a misleading error message
|
|
||
| ebool allowedTransfer = TFHE.le(amount, currentAllowance); | ||
|
|
||
| ebool canTransfer = TFHE.le(amount, _balances[owner]); |
There was a problem hiding this comment.
_transfer now handles the balance check
Original report: >>>�Hello, I think there may be an issue with the ConfidentialERC20Wrapper (https://github.com/Inco-fhevm/confidential-erc20-framework/blob/main/contracts/ConfidentialERC20Wrapper.sol).It seems possible to transfer funds while the Gateway is pending decryption (https://github.com/Inco-fhevm/confidential-erc20-framework/blob/main/contracts/ConfidentialERC20/ConfidentialERC20.sol#L247). Since the underflow is checked only when the unwrap function is called, it would be possible to steal all the ERC20 wrapped in the contract.For instance, let's say my encrypted balance is equal to 1. I call the unwrap() function with amount=1. Before the Gateway decrypts (and calls the callback function _burnCallback), I transfer my 1 to another address. Once the gateway calls the fallback, it would make my balance equal to type(uint64).max . Then, I can check the (decrypted) totalSupply and call another unwrap() passing amount = totalSupply . Once the gateway calls the callback (_burnCallback), it would transfer all the tokens to me Signed-off-by: Silas Davis <[email protected]>
silasdavis
force-pushed
the
sd/fix-burn-reentrancy
branch
from
e97c8ea to
26300f4
Compare
always freed Signed-off-by: Silas Davis <[email protected]>
Signed-off-by: Silas Davis <[email protected]>
|
|
||
| error BurnRequestDoesNotExist(uint256 requestId); | ||
|
|
||
| function _cancelBurn(uint256 requestId) internal virtual { |
There was a problem hiding this comment.
This could be exposed to the original sender/unwrapper if they wanted to cancel in-flight. We ought to be able to rely on the callback being called though, so we shouldn't need this just for cleaning up in-flight burns.
The EOA cna use the BurnRequested event to keep track of this requestId so it's possible to give them cancellation
|
If the request fails, the user will cancel it and retry? |
|
The issue with the requestBurn function is that it updates the locked balance before verifying the user has sufficient balance, causing a failure in every case. This violates the checks-effects-interactions principle. Link to relevant code - requestBurn |
Signed-off-by: Silas Davis <[email protected]>
silasdavis
force-pushed
the
sd/fix-burn-reentrancy
branch
from
9219c5c to
93c598e
Compare
Original report: